DRAFT: ocioarchive enhancements #1931
Conversation
KevinJW
added
API Change
Needs Discussion
Signed-off-by: Kevin Wheatley <[email protected]>
Signed-off-by: Kevin Wheatley <[email protected]>
…them Signed-off-by: Kevin Wheatley <[email protected]>
Needs documenting and testing May be nice to have a progress report and/or integration with OCIO Logging Signed-off-by: Kevin Wheatley <[email protected]>
If archiving a minimal config and environment variables are used, it must be possible for all the needed variables to be resolved and any files pointed to should also exist at the time of archiving. The values of the environment will be written into the saved configuration in the environment section of the configuration file. Ths configuration stored thus may be different to the initial config being passed for archiving. Needs tests adding and futher discussion in the TSC meetings. Signed-off-by: Kevin Wheatley <[email protected]>
KevinJW
force-pushed
the
ocioarchive-enhancements
branch
from
7f1a112 to
c727983
Compare
Adds default parameter value for isArchivable function to bindings. This is not matching the behaviour of the C++ API. Signed-off-by: Kevin Wheatley <[email protected]>
Signed-off-by: Kevin Wheatley <[email protected]>
MSVC thinks you need the capture, GCC thinks the capture is not needed, so pass variable directly into the lambda function call. Ugly to pass several booleans, but should work. Signed-off-by: Kevin Wheatley <[email protected]>
| @@ -1503,7 +1504,7 @@ class OCIOEXPORT Config | |||
| * | |||
| * \return bool Archivable if true. | |||
| */ | |||
| bool isArchivable() const; | |||
| bool isArchivable(bool minimal) const; | |||
There was a problem hiding this comment.
Should this accept the Flag enum instead, could make it more future proof? Adding it as an overload might be cleaner for current users of isArchivable?
There was a problem hiding this comment.
Personally I don't like the use of Flags as they can hide the actual meaning so I guess I defaulted to a single boolean, that said I'd prefer not using multiple bool's in APIs, so if we needed other parameters in the future then I'd suggest wanting to wrap the boolean in a stronger type to avoid calling function(true, false, true, ...) but if others want Flags That is simple enough
There was a problem hiding this comment.
I agree with you, although to me, even one boolean is not very explicit here, when you see isArchivable(true) somewhere randomly in the code. Flags are already used in other places of the API like for the Processor queries so that might be more consistent.
| void archive(std::ostream & ostream, ArchiveFlags flags) const; | ||
|
|
||
| //TODO: document | ||
| void GetAllFileReferences(std::set<std::string> & files) const; |
There was a problem hiding this comment.
Do we want this in the public API or is it only for OCIOZArchive need? If the latter, there might be a way to move it to ConfigUtils.h instead. If the former, I'd vote to return a vector instead of set in the spirit of reducing STL headers included here.
There was a problem hiding this comment.
That was one of my thoughts, I would think other uses for the list of files would be interesting and thus a public function may be needed.
Note that the existing code uses them as a set rather than a vector as this then makes each entry unique as it is obviously possible to use the same external file in multiple transforms in a config file (we use the same placeholder file for some of our configuration files). If we want to turn that into a vector for a public API then I guess that is fine.
There was a problem hiding this comment.
Personally it seems this new method is not strictly needed as you can implement it with the existing public API (if I'm not mistaking), but I'm not opposed to adding it if you think there is a strong benefit to have it.
Yes I think the use of set is appropriate for the implementation but not strictly required in the public API, might be nitpicking but seems slightly cleaner!
Do not merge this is for comments etc.
I've added a 'minimal mode for ocioarchive, that attempts to add only the files you need for the configuration file given.
To run you just add
--minimalto the command line.as an example here is the result of one of my configurations
The full one contains
808773530 Bytes from 246 files
the minimal one is 62104478 in 29 files
You can imagine that part of the full directory contains the ACES 1,2 configs amongst other items. and from that the minimal config only needed these
It does not yet handle environments, nor have good documentation or tests and no Python bindings
Would be good to have feedback before pursuing it further. especially as this changes the ABI and API
Kevin